-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
replicateA_, replicateM, replicateM_ #3705
Conversation
Alright: I'm not aware of any useful examples of |
Two comments:
|
If Regarding "undoing": I did not implement |
Sorry I meant the current PR has a stack unsafe replicateA_. Master does have a stack safe traverse for collections so replicate will pick that up but this implementation won't get it. As a default implementation I'd rather call void on the original value, then replicateA then void. That would be stack-safe and allow implementations that optimize void (say discard trailing maps) to leverage. |
You're sure that it won't still construct the intermediate list? I'm worried about the heap blowing up with a big |
what I suggested definitely would create the intermediate list on the heap. But the code in this PR will create the same size on the stack. The stack is much smaller than the heap, so it will blow up first. But keep in mind: it uses O(N) heap where N is the size of the object in heap now, so it is a constant multiple of heap. You might OOM, but that means you were close before. To recap, I would prefer my approach (since it will temporarily use heap, which is much greater than stack as a resource). I don't know how to implement this using O(1) heap and stack. That would be cool. I think you can probably do O(log N) stack and O(log N) heap by adapting this method:
But I don't exactly see how. That function relies on materializing the data-structure into an immutable.IndexedSeq and then recursively slicing it into a tree. So, to avoid that materialization, you need to be able to turn the original data structure into a tree. You could imagine something like: class Linear[A] {
type F
def chunk: F[A]
def recurse: Splitable[F]
}
trait Splitable[G[_]] extends Foldable[G] {
def split[A](ga: G[A], items: Int): Option[Seq[Linear[A]]]
} If you had that, you could recursively split something and walk the tree, aggregating the |
Would it be terrible to implement the current stack-unsafe method by wrapping it in I'll take a look at the current way Traverse works right now and check if that can be adapted to not create an intermediate structure. |
Ah! Using |
I ended up having an idea: Would appreciate some other eyes on this though, I'm quite likely to be wrong. |
else go(x - 1, this.productR(fa, step)) | ||
} | ||
go(n, this.pure(())) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stray closing brace:
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I'm missing an opening brace. I don't know how I didn't catch that.
@@ -144,6 +144,15 @@ import scala.annotation.implicitNotFound | |||
|
|||
tailRecM(branches.toList)(step) | |||
} | |||
|
|||
def replicateM[A](n: Int, fa: F[A]): F[List[A]] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for any lawful monad, these are exactly the same as replicateA aren't they? I am opposed to adding a function that is only different when the monad is not lawful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but it does not have the overhead of traverseViaChain
/Eval
/etc., if that would make a difference (would it?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
People can override for efficiency but my opinion is that we should not have duplicative functions that are equivalent in general.
Secondly, generally I think we should not trade stack safety for performance since lack of stack safety makes the library randomly fail when sizes get a bit larger.
I think we should have it be as fast as possible that is also stack safe, IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. Should I remove replicateM or just alias it?
Should I override replicateM with this version in StackSafeMonad?
if(x == 0) step | ||
else go(x - 1, this.productR(fa, step)) | ||
} | ||
go(n, this.pure(())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: pure(())
is the same as unit
.
nit: if we keep this implementation, I'd like to add the tailrec annotation.
instead of using fa
can we do make: val fvoid = void(fa)
outside of def go, and use that?
I still want to express this function in a stack safe way if we can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about adding @tailrec
but didn't know if it was used elsewhere. Will do.
How about moving |
Superseded by #4208. |
Spawned from a discussion about
replicateA
from Gitter.Current progress:
replicateA_
replicateM
replicateM_
replicateA_
replicateM
replicateM_
replicateA_
toApplicativeOps
replicateM
andreplicateM_
toMonadOps